Replace ITelemetryActivator with ITelemetryModule #3009
Conversation
|
This looks good to me overall. But is it possible to add some unit tests for the Also are all the failing builds of concern or transient? |
| @@ -89,7 +90,7 @@ public static ITestHost CreateJobHost( | |||
|
|
|||
| if (onSend != null) | |||
| { | |||
| serviceCollection.AddSingleton<ITelemetryActivator>(serviceProvider => | |||
| serviceCollection.AddSingleton<ITelemetryModule>(serviceProvider => | |||
There was a problem hiding this comment.
@jviau - after updating this line to register an ITelemetryModule for tests, I noticed that the lambda expression here doesn't get evaluated anymore and therefore the TelemetryActivators constructor and Intialize() method don't get called. I thought it was because the new Initialize() method takes a TelemetryConfiguration as an argument so I registered a TelemetryConfiguration as a singleton before line 91, but it's still not initializing TelemetryActivator. Do you know if there are any additional services I should register to get TelemetryActivator to register?
For context, the changes in this PR work when I manually test them in a sample DF app. The tests are just not passing because of the issue I described earlier.
There was a problem hiding this comment.
ITelemetryModule's are resolved and invoked by the AppInsights SDK. Does the test code have the AppInsights SDK hooked up?
There was a problem hiding this comment.
The test code references Microsoft.ApplicationInsights through the package reference to the WebJobs extension. It doesn't have any code to hook up Application Insights though. I tried adding services.AddApplicationInsightsTelemetry(), but that ended up giving me an error that it needs an IHostingEnvironment so I wasn't sure if that was correct.
There was a problem hiding this comment.
@jviau, were you thinking of an approach different from services.AddApplicationInsightsTelemetry()?
There was a problem hiding this comment.
Possibly? I haven't looked at the exact test failures yet, but all of this is just a convenience part of the AppInsights SDK to have a defined hook for code to run as part of the SDK starting up. If that doesn't work for your tests, doing the same thing manually -- instantiating and calling the ITelemetryModule with the correct config instance is perfectly acceptable. What that looks like exactly I can't say without taking a deeper look.
I'm working on adding unit tests for the The failing build includes some existing tests that are failing now because of the changes so I'm looking into that. I left a comment in the PR that explains the issue. |
This PR replaces
ITelemetryActivatorwithITelemetryModuleso we can have more flexibility with configuration. For example, with this change we can set Managed Identity credentials instead of only setting Application Insights relevant settings.For context:
ITelemetryActivator: Used to activate or create telemetry objects. It also initializes the telemetry client for distributed tracing, allowing settings to be verified and adjusted during initializationITelemetryModule: Used to initialize and configure telemetry modules fromTelemetryConfiguration. It ensures that telemetry modules are properly set up to collect and send data using the same configuration.Also, in this PR, we remove some log statements and checks. With
ITelemetryActivator, we checked if the customer was providingAPPINSIGHTS_INSTRUMENTATIONKEYand/orAPPLICATIONINSIGHTS_CONNECTION_STRINGand logged different warnings based on the configuration. WithITelemetryModule, it automatically has the configuration in theTelemetryConfigurationobject, so we no longer need to check for these configurations.Resolves #2870
Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.cs